General QR code scanner flow #18677
Merged
General QR code scanner flow #18677
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Jenkins BuildsClick to see older builds (74)
|
✔️ status-mobile/prs/android-e2e/PR-18677#6 🔹 ~7 min 37 sec 🔹 aa90878 🔹 📦 android-e2e package |
This was referenced Feb 7, 2024
J-Son89
reviewed
Feb 7, 2024
| [status-im.navigation.events :as navigation] | ||
| [taoensso.timbre :as log] | ||
| [utils.re-frame :as rf])) | ||
|
|
||
| (rf/defn set-new-identity-reconnected |
There was a problem hiding this comment.
can we add this new effect in it's rightful home not in legacy folder?
I understand you might need to make more changes to have the cofx in the event below, but if we can at least reduce the code added to legacy it would be great. 👍
J-Son89
reviewed
Feb 7, 2024
J-Son89
reviewed
Feb 7, 2024
J-Son89
reviewed
Feb 7, 2024
J-Son89
reviewed
Feb 7, 2024
J-Son89
reviewed
Feb 7, 2024
J-Son89
reviewed
Feb 7, 2024
J-Son89
reviewed
Feb 7, 2024
| @@ -1,13 +1,12 @@ | |||
| (ns status-im.contexts.chat.home.add-new-contact.events | |||
| (:require | |||
| [clojure.string :as string] | |||
| [re-frame.core :as re-frame] | |||
There was a problem hiding this comment.
I'm always unsure when we want to use utils.re-frame and re-frame.core.
@ilmotta, @flexsurfer is there any reasoning we have both of these options when a lot of the same logic is accessible from both??
There was a problem hiding this comment.
utils.re-frame has one defn marco which is deprecated now that's why I used re-frame.core here
There was a problem hiding this comment.
re-frame.core shouldn't be used, rf/sub and rf/dispatch should be used instead from utils.re-frame
J-Son89
reviewed
Feb 7, 2024
|
@J-Son89 ready! |
jo-mut
approved these changes
Feb 9, 2024
| (load-and-show-profile scanned-text) | ||
|
|
||
| (eth-address? scanned-text) | ||
| (debounce/debounce-and-dispatch [:navigate-to :wallet-accounts address] 300) |
There was a problem hiding this comment.
i think for the wallet the user navigates to the wallet tab. Here you can make use of navigate-change-tab instead of navigate-to to launch the wallet tab once the user scans the wallet qr-code
(rf/dispatch [:navigate-change-tab :wallet-stack])or maybe this could be another screen that looks exactly like the wallet-tab and we could use navigate-to
There was a problem hiding this comment.
its actually the home - wallet tab
33% of end-end tests have passedFailed tests (31)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePR:
Class TestDeepLinksOneDevice:
Class TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityMultipleDeviceMerged:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (16)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
|
J-Son89
approved these changes
Feb 9, 2024
|
Hi @alwx could you please rebase and resolve conflicts for the current PR? Thanx! |
54% of end-end tests have passedFailed tests (21)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePRTwo:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityMultipleDeviceMerged:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (26)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestDeepLinksOneDevice:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityMultipleDeviceMerged:
|
|
Hi @alwx, thank you for the fixes. I especially appreciate you for implementing things that weren't originally in scope. Could you please take a look at the additional issues? ISSUE 2: [IOS] The "nx().isAddress is not a function" error is shown when the eth address is scanned from different resources or QR with URLSteps:
Metamask QR with URL Actual Result:The "nx().isAddress is not a function" error is displayed. Expected Result:No error should occur when scanning an ETH address from a different resource. Device:real device: iPhone 11 Pro max, IOS 17 Logs: |
ISSUE 3: [Android] The old wallet screen is opened when scanned QR code is generated from different resourcesSteps:Scan the QR from https://brunobar79.github.io/eip681-link-generator/# which include chainid and amount of tokens Actual result:The old screen wallet is opened oldwallet.mp4Expected result:The new screen wallet is opened Device:Pixel 7a, Android 14 Logs: |
actually if you look at the designs, it should open a bottom sheet with the option to save-address (not completed yet anyway) or to send 
If this is the case I personally would recommend just adding the bottom sheet here but leave the functionality with regards to send and save address to a follow up pr as there is quite a bit involved in setting that up correctly and would be good to align with @briansztamfater before implementing 👍 |
|
@VolodLytvynenko I don't see the |
@alwx I am also reproducing the bug reported by @VolodLytvynenko I am using latest IOS PR build #17 Are you sure you committed latest changes in this PR? Are you checking on IOS device? This is IOS only bug. Android is okay as far as I understand, correct @VolodLytvynenko? telegram-cloud-document-2-5361981410213116083.mp4 |
@pavloburykh That's correct, thank you for checking. The error is shown only for IOS devices. For Android, I have created issue 3 separatly |
|
@VolodLytvynenko fixed & ready to be checked |
|
After an investigation I figured that |
25% of end-end tests have passedFailed tests (35)Click to expandClass TestActivityMultipleDevicePRTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Class TestDeepLinksOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (12)Click to expandClass TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestDeepLinksOneDevice:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
|
|
hi @alwx, thank you for the fixes. PR is ready to be merged |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #17574
fixes #14011
Summary
Implements general QR scanner, and in addition to that make it work with profile links and wallet links. However, due to the fact that the ability to open a wallet with a specific address is not implemented in Status yet (you can only see information about a wallet you started "watching"), opening a wallet address just opens the wallet screen and doesn't load the information about it.
Profile links can be in several formats:
https://status.app/u/<address>(works with other domains & other url schemas as well)<address>(both compressed and non-compressed formats are supported)Figma:
https://www.figma.com/file/eDfxTa9IoaCMUy5cLTp0ys/Shell-for-Mobile?type=design&node-id=451-18994&mode=design&t=7NiEy7z2dlFCxj6P-0
Follow up issues:
Platforms
status: ready